Remove allocations in ErorrFacts #75234
Conversation
When profiling our unit tests I discovered that `ErrorFacts` are a significant source of `string` allocations. In `Emit` for example it can be over 200MB of allocations. This is due to our use of `Enum.ToString` as well as concat that value with well known constants. To avoid these allocations I changed our code generator to produce these strings at compile time. I was a bit concerned about the performance of the `switch` compared to the existing code so I wrote a quick benchmark to demonstrate that there is no perf lost here. | Method | Runtime | Mean | Ratio | Gen0 | Allocated | Alloc Ratio | |------------------- |--------------------- |----------:|------:|--------:|----------:|------------:| | EnumToString | .NET 8.0 | 66.80 us | 1.00 | - | 45046 B | 1.00 | | EnumToStringHelper | .NET 8.0 | 15.76 us | 0.24 | - | - | 0.00 | | EnumToString | .NET Framework 4.7.2 | 512.30 us | 7.69 | 21.4844 | 140197 B | 3.11 | | EnumToStringHelper | .NET Framework 4.7.2 | 17.70 us | 0.27 | - | - | 0.00 |
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
@dotnet/roslyn-compiler PTAL |
| @@ -8129,7 +8129,7 @@ void binaryOperator(string op, string leftType, string rightType, string expecte | |||
|
|
|||
| if (expectedDiagnostics.Length == 0) | |||
| { | |||
| CompileAndVerify(comp, verify: Verification.FailsPEVerify); | |||
| CompileAndVerify(comp, verify: Verification.FailsPEVerify, emitOptions: EmitOptions.Default); | |||
There was a problem hiding this comment.
This is a different perf fix I was working on that I accidentally included with this change. I'll revert this part
I am not sure what is the common pattern for using this API, but it looks like our resources contain only 20 ids like that and all of them are for warnings. Refers to: src/Compilers/CSharp/Portable/Generated/ErrorFacts.Generated.cs:4157 in cbcb2e5. [](commit_id = cbcb2e5, deletion_comment = False) |
|
Do we understand why tests are requesting the strings? Would it be possible to decrease the allocation just by making changes to the test infrastructure? |
|
If we go with the change, we probably should adjust the following comment at the end of |
|
I am not sure that we are optimizing for a common real-world scenario, but we are definitely paying with more memory usage by the compiler (string heap, code size, etc.) for all scenarios. |
The primary method that causes issues is
The places I've dug into need to realize the string in some way to do their job.
I agree that is a consideration we should make. The The |
I dug into this a bit more and wrote a program to dump the
Looking at that I agree that the price we're paying for |
|
I dug into this some more. The reason that there was not a significant increase in the user string heap here is that because the majority of these strings already existed there. This is because of our .resx generation pass where these values, in a longer form, were placed on the user string heap already. That meant this was a roughly net neutral change for the However the method that is generated by this code is extremely large and significantly increases the size of the resulting binary. The size increase simply isn't worth it given this problem is largely about testing, not product scenarios. As such abandoning this approach for now. May revisit later with a more targeted approach. |
|
It would be nice if the ErrorCode member names were laid out contiguously in some way we could rely on and we could literally index into them. |
When profiling our unit tests I discovered that
ErrorFactsare a significant source ofstringallocations. InEmitfor example it can be over 200MB of allocations.This is due to our use of
Enum.ToStringas well as concat that value with well known constants. To avoid these allocations I changed our code generator to produce these strings at compile time.I was a bit concerned about the performance of the
switchcompared to the existing code so I wrote a quick benchmark to demonstrate that there is no perf lost here.